Feat/crystals+anchors#31
Open
HashimTheArab wants to merge 34 commits into
Open
Conversation
- Implement end crystals as behavior - Add EndCrystal to explosion config
Constraint: PR df-mc#1247 had merge conflicts against upstream/master in block registration, projectile behavior, and sound declarations. Rejected: Dropping upstream redstone/piercing changes | conflict resolution must preserve current base behavior. Confidence: high Scope-risk: moderate Directive: Keep projectile piercing collision tracking and non-living damageable entities in sync when editing ProjectileBehaviour. Tested: go test ./...; git diff --check Not-tested: In-game Bedrock client interaction.
End crystal placement and damage behavior needed source-backed parity before PR df-mc#1247 could be reviewed cleanly. This keeps the fixes isolated from the upstream merge already pushed to the contributor branch. Constraint: Primary source guidance came from ../AGENTS.md: Minecraft Wiki and mcsrc.dev first, with Dragonfly conventions preserved where code patterns already existed. Rejected: Keeping duplicate non-living damage helpers | player and projectile paths both need the same Behaviour-backed damage semantics. Confidence: high Scope-risk: moderate Directive: Keep End crystal zero-damage, explosion-damage, placement-fire, and continuous-End-fire behavior covered when changing entity or item placement paths. Tested: go test ./... Not-tested: Live Bedrock client interaction; dedicated regression tests per requester direction.
…fixes Fix source-backed crystal and anchor parity issues
Constraint: PR df-mc#1247 had already merged before this review cleanup, so this follow-up keeps the change limited to the offset declaration. Rejected: Keeping generated offset initialization | the explicit priority table is clearer and avoids an init-time helper for fixed vanilla order. Confidence: high Scope-risk: narrow Directive: Preserve the column layer-1/layer-2 priority before layer 3 when editing respawn anchor spawn offsets. Tested: go test ./...; git diff --check Not-tested: Live Bedrock client respawn flow.
Constraint: PR df-mc#1247 follow-up review requested documentation for helper tables and questioned whether a missing End crystal beam target should serialize as 0,0,0. Rejected: Sending a zero BlockTarget for crystals without a beam target | zero is a concrete position and can be interpreted as a beam target. Confidence: high Scope-risk: narrow Directive: Omit EntityDataKeyBlockTarget unless an End crystal has an explicit beam target. Tested: go test ./...; git diff --check Not-tested: Live Bedrock client metadata rendering.
Constraint: PR df-mc#1247 follow-up review requested documentation for helper tables and questioned whether a missing End crystal beam target should serialize as 0,0,0. Rejected: Sending a zero BlockTarget for crystals without a beam target | zero is a concrete position and can be interpreted as a beam target. Confidence: high Scope-risk: narrow Directive: Omit EntityDataKeyBlockTarget unless an End crystal has an explicit beam target. Tested: go test ./...; git diff --check Not-tested: Live Bedrock client metadata rendering.
Constraint: Follow-up PR review requested clarity on HurtEntity return values without changing behavior. Rejected: Adding explicit ProjectileDamageSource ExplodesEndCrystal true | End crystal explosion behavior is opt-out by default, so explicit true methods add noise. Confidence: high Scope-risk: narrow Directive: Keep HurtEntity return tuple semantics aligned with Living.Hurt plus the damageable ok flag. Tested: go test ./...; git diff --check Not-tested: Live Bedrock client interaction.
Constraint: Respawn anchors and beds share the same safe-spawn contract in player respawn logic. Rejected: Keeping the anonymous inline interface | It obscures the reusable local contract and makes the respawn path harder to scan. Confidence: high Scope-risk: narrow Directive: Keep this interface private unless a non-player package needs to refer to the contract directly. Tested: go test ./...; git diff --check Not-tested: Live Bedrock client respawn interaction.
Constraint: Bedrock lang sources keep tile.bed.notValid scoped to bed wording while respawn anchors have their own notValid key. Rejected: Java-style combined bed and anchor fallback | It does not match Dragonfly's original fallback or current Bedrock sample text. Confidence: high Scope-risk: narrow Directive: Keep chat translation fallbacks aligned with Bedrock language keys, not wiki prose when they differ. Tested: go test ./...; git diff --check Not-tested: Live Bedrock client locale rendering.
📝 WalkthroughWalkthroughThis PR adds End Crystal entities/items, Respawn Anchor blocks with dimension-aware player spawn persistence, Bamboo wood block variants, a graded BoneMealResult enum replacing boolean bone-meal returns across crop blocks, projectile piercing with a generalized HurtEntity abstraction, plus debug shapes, chunk/palette cloning, and assorted fixes. ChangesEnd Crystal Entity and Item
Respawn Anchor Block and Dimension-Aware Player Spawn
Bamboo Wood Blocks
BoneMealResult Refactor
Projectile Piercing and Generalized Entity Damage
Debug Shapes, Chunk Cloning, and Misc
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant World
participant Provider
participant DB
Player->>World: SetPlayerSpawn(pos)
World->>Provider: SavePlayerSpawn(PlayerSpawn{Pos, Dim})
Provider->>DB: SavePlayerSpawn(id, spawn)
Player->>World: PlayerSpawnPoint(id)
World->>Provider: LoadPlayerSpawn(id)
Provider->>DB: LoadPlayerSpawn(id)
DB-->>World: PlayerSpawn{Pos, Dim}
World-->>Player: spawn, ok
sequenceDiagram
participant Projectile
participant Entity
participant HurtEntity
Projectile->>Entity: trace hit (ignoring collided entities)
Projectile->>HurtEntity: HurtEntity(entity, damage, source)
HurtEntity-->>Projectile: n, vulnerable, ok
Projectile->>Projectile: record entity in collidedEntities
alt collided count > PiercingLevel
Projectile->>Projectile: close entity
else
Projectile->>Projectile: continue flight
end
Related PRs: None identified. Suggested labels: enhancement, feature, refactor Suggested reviewers: None identified. 🐰 A crystal hums with End-lit fire, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6f5385b. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/entity/projectile.go`:
- Around line 179-183: The collision bookkeeping and hit handling are
incorrectly gated on lt.conf.Damage >= 0 which prevents non-damaging projectiles
from registering hits; remove the Damage >= 0 guard so that
lt.hitEntity(r.Entity(), e, vel) is called on entity collision for all
projectiles and ensure lt.collidedEntities = append(lt.collidedEntities,
r.Entity().H()) is executed when damageableEntity(r.Entity()) is true regardless
of lt.conf.Damage; apply the same change to the other similar blocks that
reference lt.conf.Damage, lt.hitEntity, lt.collidedEntities, and
damageableEntity so collisions are always recorded even for negative-damage
configs.
In `@server/entity/register.go`:
- Around line 52-56: The Arrow spawn function currently does an unchecked type
assertion tip := arrow.Tip.(potion.Potion) and directly calls arrow.Owner.H(),
which can panic if Tip is nil/unset or Owner is nil; update the Arrow function
to defensively check arrow.Tip's type (use a type assertion with ok or a nil
check) before assigning to tip and fall back to a safe default potion or leave
conf.Potion unset, and also validate arrow.Owner is non-nil before calling
arrow.Owner.H() (or use a safe owner handle getter); ensure you still set
conf.Damage, conf.KnockBackForceAddend, and other fields only after these checks
so spawn-time panics are prevented.
In `@server/player/player.go`:
- Around line 1793-1803: The branch for non-living entities returns before the
item's durability is updated; when entity.HurtEntity(e, i.AttackDamage(), ...)
succeeds you must invoke the same durability-wear logic used for living targets
before returning true. Modify the non-living branch (the block using
entity.HurtEntity, p.tx.PlaySound and p.Exhaust) to call the existing
item-durability handler/mutation that the living-target path uses (i.e., the
same code that updates the item stack and triggers break/consume) so durable
items are decremented on successful hits against non-living entities as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf686165-893e-4c43-90d8-aaab341ed2a3
📒 Files selected for processing (30)
server/block/blast_furnace.goserver/block/explosion.goserver/block/furnace.goserver/block/hash.goserver/block/register.goserver/block/respawn_anchor.goserver/block/smoker.goserver/entity/damage.goserver/entity/damageable.goserver/entity/end_crystal.goserver/entity/end_crystal_behaviour.goserver/entity/projectile.goserver/entity/register.goserver/item/bow.goserver/item/crossbow.goserver/item/enchantment/multishot.goserver/item/enchantment/piercing.goserver/item/enchantment/register.goserver/item/end_crystal.goserver/item/golden_apple.goserver/item/item.goserver/item/register.goserver/item/stack.goserver/player/chat/translate.goserver/player/player.goserver/session/entity_metadata.goserver/session/world.goserver/world/block_registry.goserver/world/entity.goserver/world/sound/block.go
Comment on lines
+179
to
+183
| if lt.conf.Damage >= 0 { | ||
| lt.hitEntity(r.Entity(), e, vel) | ||
| if damageableEntity(r.Entity()) { | ||
| lt.collidedEntities = append(lt.collidedEntities, r.Entity().H()) | ||
| } |
There was a problem hiding this comment.
Non-damaging projectiles no longer terminate on entity collision
Line 179 gates collision bookkeeping behind Damage >= 0, but Line 200 closes using collidedEntities length. For configs using negative damage, hits are never counted, so the projectile never closes after entity impact (and with Line 333 it can just stop moving in-place).
🔧 Proposed fix
switch r := result.(type) {
case trace.EntityResult:
- if lt.conf.Damage >= 0 {
- lt.hitEntity(r.Entity(), e, vel)
- if damageableEntity(r.Entity()) {
- lt.collidedEntities = append(lt.collidedEntities, r.Entity().H())
- }
- }
+ if damageableEntity(r.Entity()) {
+ if lt.conf.Damage >= 0 {
+ lt.hitEntity(r.Entity(), e, vel)
+ }
+ lt.collidedEntities = append(lt.collidedEntities, r.Entity().H())
+ }Also applies to: 200-202, 333-335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/entity/projectile.go` around lines 179 - 183, The collision
bookkeeping and hit handling are incorrectly gated on lt.conf.Damage >= 0 which
prevents non-damaging projectiles from registering hits; remove the Damage >= 0
guard so that lt.hitEntity(r.Entity(), e, vel) is called on entity collision for
all projectiles and ensure lt.collidedEntities = append(lt.collidedEntities,
r.Entity().H()) is executed when damageableEntity(r.Entity()) is true regardless
of lt.conf.Damage; apply the same change to the other similar blocks that
reference lt.conf.Damage, lt.hitEntity, lt.collidedEntities, and
damageableEntity so collisions are always recorded even for negative-damage
configs.
Comment on lines
+52
to
+56
| Arrow: func(opts world.EntitySpawnOpts, arrow world.ArrowSpawnConfig) *world.EntityHandle { | ||
| tip := arrow.Tip.(potion.Potion) | ||
| conf := arrowConf | ||
| conf.Damage, conf.Potion, conf.Owner = damage, tip.(potion.Potion), owner.H() | ||
| conf.KnockBackForceAddend = float64(punchLevel) * enchantment.Punch.KnockBackMultiplier() | ||
| conf.DisablePickup = disallowPickup | ||
| if obtainArrowOnPickup { | ||
| conf.PickupItem = item.NewStack(item.Arrow{Tip: tip.(potion.Potion)}, 1) | ||
| conf.Damage, conf.Potion, conf.Owner = arrow.Damage, tip, arrow.Owner.H() | ||
| conf.KnockBackForceAddend = float64(arrow.PunchLevel) * enchantment.Punch.KnockBackMultiplier() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all ArrowSpawnConfig callsites and whether Tip/Owner are always set.
fd -e go | xargs rg -nP 'ArrowSpawnConfig\s*{' -C3
fd -e go | xargs rg -nP 'EntityRegistry\(\)\.Config\(\)\.Arrow\s*\(' -C3Repository: HashimTheArab/dragonfly
Length of output: 822
Defensively handle world.ArrowSpawnConfig fields to avoid spawn-time panics.
server/entity/register.go does tip := arrow.Tip.(potion.Potion), but server/item/crossbow.go constructs world.ArrowSpawnConfig{...} without setting Tip, so this can panic when spawning arrows. Guard the type assertion (and arrow.Owner.H() for safety).
💡 Suggested hardening
Arrow: func(opts world.EntitySpawnOpts, arrow world.ArrowSpawnConfig) *world.EntityHandle {
- tip := arrow.Tip.(potion.Potion)
+ tip, _ := arrow.Tip.(potion.Potion)
conf := arrowConf
- conf.Damage, conf.Potion, conf.Owner = arrow.Damage, tip, arrow.Owner.H()
+ conf.Damage, conf.Potion = arrow.Damage, tip
+ if arrow.Owner != nil {
+ conf.Owner = arrow.Owner.H()
+ }
conf.KnockBackForceAddend = float64(arrow.PunchLevel) * enchantment.Punch.KnockBackMultiplier()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Arrow: func(opts world.EntitySpawnOpts, arrow world.ArrowSpawnConfig) *world.EntityHandle { | |
| tip := arrow.Tip.(potion.Potion) | |
| conf := arrowConf | |
| conf.Damage, conf.Potion, conf.Owner = damage, tip.(potion.Potion), owner.H() | |
| conf.KnockBackForceAddend = float64(punchLevel) * enchantment.Punch.KnockBackMultiplier() | |
| conf.DisablePickup = disallowPickup | |
| if obtainArrowOnPickup { | |
| conf.PickupItem = item.NewStack(item.Arrow{Tip: tip.(potion.Potion)}, 1) | |
| conf.Damage, conf.Potion, conf.Owner = arrow.Damage, tip, arrow.Owner.H() | |
| conf.KnockBackForceAddend = float64(arrow.PunchLevel) * enchantment.Punch.KnockBackMultiplier() | |
| Arrow: func(opts world.EntitySpawnOpts, arrow world.ArrowSpawnConfig) *world.EntityHandle { | |
| tip, _ := arrow.Tip.(potion.Potion) | |
| conf := arrowConf | |
| conf.Damage, conf.Potion = arrow.Damage, tip | |
| if arrow.Owner != nil { | |
| conf.Owner = arrow.Owner.H() | |
| } | |
| conf.KnockBackForceAddend = float64(arrow.PunchLevel) * enchantment.Punch.KnockBackMultiplier() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/entity/register.go` around lines 52 - 56, The Arrow spawn function
currently does an unchecked type assertion tip := arrow.Tip.(potion.Potion) and
directly calls arrow.Owner.H(), which can panic if Tip is nil/unset or Owner is
nil; update the Arrow function to defensively check arrow.Tip's type (use a type
assertion with ok or a nil check) before assigning to tip and fall back to a
safe default potion or leave conf.Potion unset, and also validate arrow.Owner is
non-nil before calling arrow.Owner.H() (or use a safe owner handle getter);
ensure you still set conf.Damage, conf.KnockBackForceAddend, and other fields
only after these checks so spawn-time panics are prevented.
Comment on lines
1793
to
1803
| if !isLiving { | ||
| return false | ||
| n, vulnerable, ok := entity.HurtEntity(e, i.AttackDamage(), entity.AttackDamageSource{Attacker: p}) | ||
| if !ok { | ||
| return false | ||
| } | ||
| p.tx.PlaySound(entity.EyePosition(e), sound.Attack{Damage: !mgl64.FloatEqual(n, 0)}) | ||
| if vulnerable { | ||
| p.Exhaust(0.1) | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Durability isn’t consumed when attacking non-living entities.
This new branch returns before the durability logic, so durable items never wear when damaging non-living targets.
Proposed fix
if !isLiving {
n, vulnerable, ok := entity.HurtEntity(e, i.AttackDamage(), entity.AttackDamageSource{Attacker: p})
if !ok {
return false
}
+ if durable, ok := i.Item().(item.Durable); ok {
+ _, left := p.HeldItems()
+ p.SetHeldItems(p.damageItem(i, durable.DurabilityInfo().AttackDurability), left)
+ }
p.tx.PlaySound(entity.EyePosition(e), sound.Attack{Damage: !mgl64.FloatEqual(n, 0)})
if vulnerable {
p.Exhaust(0.1)
}
return true
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/player/player.go` around lines 1793 - 1803, The branch for non-living
entities returns before the item's durability is updated; when
entity.HurtEntity(e, i.AttackDamage(), ...) succeeds you must invoke the same
durability-wear logic used for living targets before returning true. Modify the
non-living branch (the block using entity.HurtEntity, p.tx.PlaySound and
p.Exhaust) to call the existing item-durability handler/mutation that the
living-target path uses (i.e., the same code that updates the item stack and
triggers break/consume) so durable items are decremented on successful hits
against non-living entities as well.
Constraint: Respawn anchors must resolve in their saved dimension, End crystal placement checks the two-block column above the base, End crystal block clipping is source-backed only for supported crystals, and water must suppress End crystal entity impact. Rejected: Keeping position-only player spawn provider methods | Spawn position without dimension cannot model Bedrock's saved respawn target once anchors are supported. Confidence: medium Scope-risk: moderate Directive: Keep player spawn dimension persistence aligned with Bedrock player data and do not reapply End crystal Y clipping to entity exposure without a primary source. Tested: go test ./...; git diff --check Not-tested: Live Bedrock client respawn across dimensions; in-game End crystal water neutralization.
Fix respawn and End crystal parity gaps
…player has no suitable item to insert projectile into it (df-mc#1272) * crossbow.go: Fix crossbow charging animation played even if the player has no suitable item to insert suitable projectile into it * crossbow.go: use blank identifier for unused "tx" variable
* Update session.go * Update session.go
…mc#1278) * fix(session): guard nil session handle in metadata * fix(session): guard nil entity handles in session helpers * Update session_list.go * various changes * Update session.go
* Implement bamboo building set * Order bamboo item registrations * Improve documentation for allWood function Updated the allWood function documentation to clarify that bamboo blocks are registered separately. * avoid duplicate bamboo block properties
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_9afaf66c-c28f-49ea-911b-bab5c9e10a1e) |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/block/sugar_cane.go (1)
67-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn
BoneMealResultNonewhen no sugar cane block is placed.
canGrowHereonly checks the base conditions, so a max-height cane stack still reportsBoneMealResultSmalleven though the loop doesn't add anything. That makes bone meal get consumed and the growth particle play on a no-op.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/block/sugar_cane.go` around lines 67 - 80, Update SugarCane.BoneMeal so it only returns BoneMealResultSmall when at least one SugarCane block is actually placed. After the canGrowHere check and placement loop, track whether any Air block was replaced via tx.SetBlock, and return BoneMealResultNone when the loop makes no changes (for example, a max-height stack). Keep the existing logic in SugarCane.BoneMeal and canGrowHere, but gate the success result on an actual placement.server/block/sea_pickle.go (1)
45-78: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn
BoneMealResultAreafor sea pickle spread.
This loop places pickles across a multi-block area, soSmallunderreports the effect and triggers the smaller particle burst.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/block/sea_pickle.go` around lines 45 - 78, Update SeaPickle.BoneMeal to return item.BoneMealResultArea instead of item.BoneMealResultSmall when the spread loop succeeds, since this method is placing new SeaPickle blocks across multiple positions and should report an area effect; keep the existing placement logic and only change the final result returned from BoneMeal.server/block/respawn_anchor.go (1)
137-156: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCandidate spawn positions aren't checked for lava/water.
respawnAnchorSpawnClearonly rejects a position when a block's model bbox overlaps the 2‑block column; liquids typically have empty collision models, so lava/water at a candidate position won't be detected as an obstruction. Vanilla explicitly treats liquids as obstructing: if the area around it is unsuitable for respawning (i.e Lava surrounding it), a message is displayed reading "You have no home bed or charged respawn anchor, or it was obstructed".Bed.Activatealready guards against this with an explicittx.Liquid(headPos)check, but that check isn't mirrored here for anchor candidates.🛡️ Proposed fix
occupied := cube.Box(0, 0, 0, 1, 2, 1).Translate(pos.Vec3()) for y := 0; y < 2; y++ { blockPos := pos.Add(cube.Pos{0, y}) + if _, ok := tx.Liquid(blockPos); ok { + return false + } for _, box := range tx.Block(blockPos).Model().BBox(blockPos, tx) { if box.Translate(blockPos.Vec3()).IntersectsWith(occupied) { return false } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/block/respawn_anchor.go` around lines 137 - 156, respawnAnchorSpawnClear currently only checks block collision boxes, so liquid blocks like lava or water can still be treated as valid spawn spaces. Update this function to also reject candidate positions when either the target block or the block above contains liquid, using the existing world liquid lookup on tx for the checked positions. Keep the existing bounds and solid-support checks, and make the new liquid obstruction check part of the same respawn clearance logic in respawnAnchorSpawnClear so it matches Bed.Activate behavior.
🧹 Nitpick comments (4)
server/item/end_crystal.go (2)
25-27: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePlacement only allows strict air, not other replaceable blocks.
Vanilla allows placement if the two blocks above the bedrock or obsidian block are air or replaceable blocks and no other entities intersect the area. This implementation only permits exact air, rejecting placement over e.g. short grass, snow layers, or non-solid replaceable vegetation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/item/end_crystal.go` around lines 25 - 27, The placement check in EndCrystal placement is too strict because it only accepts exact air for the two blocks above the base block. Update the logic in the EndCrystal placement path to use the same replaceable-block check as vanilla, allowing air or other replaceable blocks (such as grass, snow layers, or vegetation) while still rejecting non-replaceable blocks and respecting entity collision checks.
16-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer type assertion over string comparison for base-block validation.
Using
EncodeBlock()string comparison works, but a type assertion (e.g._, obsidian := tx.Block(pos).(block.Obsidian)) is more idiomatic here and avoids depending on the exact NBT/name-encoding format, consistent with block-type checks used elsewhere in the codebase (e.g.RespawnAnchor.Activate'sheld.Item().(Glowstone)).♻️ Suggested refactor
- clickedBlock, _ := tx.Block(pos).EncodeBlock() - if clickedBlock != "minecraft:obsidian" && clickedBlock != "minecraft:bedrock" { + switch tx.Block(pos).(type) { + case Obsidian, Bedrock: + default: return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/item/end_crystal.go` around lines 16 - 19, The base-block check in the crystal placement logic is using EncodeBlock string comparisons instead of a block type check. Update the validation in the end crystal handler to use type assertions on tx.Block(pos), matching the style used elsewhere such as RespawnAnchor.Activate and other block-specific checks, and accept only the Obsidian/Bedrock block types rather than relying on encoded names.server/block/carrot.go (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating duplicated growth-bonemeal logic.
The
BoneMealimplementation here is structurally identical toPotato.BoneMeal(server/block/potato.go) andWheatSeeds.BoneMeal(server/block/wheat_seeds.go) — only the receiver type differs. A small shared helper (e.g. on the embeddedcroptype, taking the concrete block as a parameter) could remove this triplication now that all three were touched in this refactor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/block/carrot.go` around lines 41 - 47, The BoneMeal logic in Carrot is duplicated across Carrot.BoneMeal, Potato.BoneMeal, and WheatSeeds.BoneMeal; extract the shared growth-and-set behavior into a common helper on the embedded crop type or a small shared function. Update the Carrot, Potato, and WheatSeeds implementations to call that helper with the concrete block receiver so the growth update, tx.SetBlock call, and BoneMealResult handling stay identical in one place.server/player/player.go (1)
949-1003: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated portal-destination fallback computation.
fallback := p.tx.World().PortalDestination(p.tx.World().Dimension())re-implements the same "round-trip portal" pattern already used insidespawnLocation()'s no-spawn-point branch (w = w.PortalDestination(w.Dimension())). Consider extracting a small helper (e.g.overworldFallback(w *world.World) *world.World) shared by both call sites to avoid the two places drifting apart later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/player/player.go` around lines 949 - 1003, The respawn flow duplicates the portal-destination fallback logic already present in spawnLocation(), which can drift over time. Extract the shared world fallback calculation into a small helper such as overworldFallback and use it both in Player.respawn and spawnLocation()’s no-spawn-point branch. Keep the helper centered around PortalDestination and Dimension so both call sites stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/item/crossbow.go`:
- Around line 158-162: The Crossbow charging check in CanCharge is using the
wrong item state, so the charging path never starts after Player.UseItem falls
back from ReleaseCharge. Update CanCharge in Crossbow to require the crossbow to
be empty instead of non-empty, while still keeping the projectile lookup via
findProjectile and the found check.
In `@server/world/mcdb/db.go`:
- Around line 75-79: The spawn coordinate loading in the db.go path is using
unsafe type assertions for SpawnX, SpawnY, and SpawnZ, so malformed server data
can panic. Update the spawn parsing logic around the existing serverData lookup
and PlayerSpawn construction to use guarded comma-ok assertions, matching the
safer SpawnDimension handling already used nearby, and return the same error
path if any coordinate is missing or not int32.
---
Outside diff comments:
In `@server/block/respawn_anchor.go`:
- Around line 137-156: respawnAnchorSpawnClear currently only checks block
collision boxes, so liquid blocks like lava or water can still be treated as
valid spawn spaces. Update this function to also reject candidate positions when
either the target block or the block above contains liquid, using the existing
world liquid lookup on tx for the checked positions. Keep the existing bounds
and solid-support checks, and make the new liquid obstruction check part of the
same respawn clearance logic in respawnAnchorSpawnClear so it matches
Bed.Activate behavior.
In `@server/block/sea_pickle.go`:
- Around line 45-78: Update SeaPickle.BoneMeal to return item.BoneMealResultArea
instead of item.BoneMealResultSmall when the spread loop succeeds, since this
method is placing new SeaPickle blocks across multiple positions and should
report an area effect; keep the existing placement logic and only change the
final result returned from BoneMeal.
In `@server/block/sugar_cane.go`:
- Around line 67-80: Update SugarCane.BoneMeal so it only returns
BoneMealResultSmall when at least one SugarCane block is actually placed. After
the canGrowHere check and placement loop, track whether any Air block was
replaced via tx.SetBlock, and return BoneMealResultNone when the loop makes no
changes (for example, a max-height stack). Keep the existing logic in
SugarCane.BoneMeal and canGrowHere, but gate the success result on an actual
placement.
---
Nitpick comments:
In `@server/block/carrot.go`:
- Around line 41-47: The BoneMeal logic in Carrot is duplicated across
Carrot.BoneMeal, Potato.BoneMeal, and WheatSeeds.BoneMeal; extract the shared
growth-and-set behavior into a common helper on the embedded crop type or a
small shared function. Update the Carrot, Potato, and WheatSeeds implementations
to call that helper with the concrete block receiver so the growth update,
tx.SetBlock call, and BoneMealResult handling stay identical in one place.
In `@server/item/end_crystal.go`:
- Around line 25-27: The placement check in EndCrystal placement is too strict
because it only accepts exact air for the two blocks above the base block.
Update the logic in the EndCrystal placement path to use the same
replaceable-block check as vanilla, allowing air or other replaceable blocks
(such as grass, snow layers, or vegetation) while still rejecting
non-replaceable blocks and respecting entity collision checks.
- Around line 16-19: The base-block check in the crystal placement logic is
using EncodeBlock string comparisons instead of a block type check. Update the
validation in the end crystal handler to use type assertions on tx.Block(pos),
matching the style used elsewhere such as RespawnAnchor.Activate and other
block-specific checks, and accept only the Obsidian/Bedrock block types rather
than relying on encoded names.
In `@server/player/player.go`:
- Around line 949-1003: The respawn flow duplicates the portal-destination
fallback logic already present in spawnLocation(), which can drift over time.
Extract the shared world fallback calculation into a small helper such as
overworldFallback and use it both in Player.respawn and spawnLocation()’s
no-spawn-point branch. Keep the helper centered around PortalDestination and
Dimension so both call sites stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4da33e28-672b-44ed-97e9-b698edd01cae
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (68)
go.modserver/block/bamboo_block.goserver/block/bamboo_mosaic.goserver/block/bed.goserver/block/beetroot_seeds.goserver/block/carrot.goserver/block/cocoa_bean.goserver/block/cube/trace/bbox.goserver/block/cube/trace/block.goserver/block/double_flower.goserver/block/fern.goserver/block/flower.goserver/block/grass.goserver/block/hash.goserver/block/kelp.goserver/block/log.goserver/block/melon_seeds.goserver/block/pink_petals.goserver/block/potato.goserver/block/pumpkin_seeds.goserver/block/register.goserver/block/respawn_anchor.goserver/block/sea_pickle.goserver/block/short_grass.goserver/block/slab.goserver/block/slab_type.goserver/block/stairs.goserver/block/stairs_type.goserver/block/sugar_cane.goserver/block/wheat_seeds.goserver/block/wood.goserver/block/wood_type.goserver/entity/end_crystal_behaviour.goserver/entity/projectile.goserver/item/bone_meal.goserver/item/creative/creative_items.nbtserver/item/crossbow.goserver/item/end_crystal.goserver/item/item.goserver/item/recipe/crafting_data.nbtserver/item/recipe/potion_data.nbtserver/player/bossbar/colour.goserver/player/chat/translate.goserver/player/conf.goserver/player/debug/shape.goserver/player/player.goserver/server.goserver/session/enchantment_texts.goserver/session/entity_metadata.goserver/session/player.goserver/session/session.goserver/session/text.goserver/session/world.goserver/world/biome/register.goserver/world/biome/sulfur_caves.goserver/world/block_states.nbtserver/world/chunk/block_registry.goserver/world/chunk/chunk.goserver/world/chunk/palette.goserver/world/chunk/paletted_storage.goserver/world/chunk/sub_chunk.goserver/world/mcdb/db.goserver/world/mcdb/leveldat/data.goserver/world/mcdb/leveldat/level_dat.goserver/world/particle/block.goserver/world/provider.goserver/world/vanilla_items.nbtserver/world/world.go
✅ Files skipped from review due to trivial changes (2)
- server/session/enchantment_texts.go
- server/block/hash.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/session/entity_metadata.go
- server/player/chat/translate.go
- server/entity/projectile.go
Comment on lines
+158
to
+162
| // CanCharge ... | ||
| func (c Crossbow) CanCharge(releaser Releaser, _ *world.Tx, ctx *UseContext) bool { | ||
| _, found := c.findProjectile(releaser, ctx) | ||
| return found && !c.Item.Empty() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the crossbow implementation and nearby call sites.
git ls-files server/item/crossbow.go server/** | sed -n '1,120p'
echo '--- crossbow.go ---'
cat -n server/item/crossbow.go | sed -n '1,260p'
echo '--- search ReleaseCharge / CanCharge / UseItem ---'
rg -n "ReleaseCharge|CanCharge|UseItem|findProjectile" server -SRepository: HashimTheArab/dragonfly
Length of output: 17644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n server/player/player.go | sed -n '1548,1578p'Repository: HashimTheArab/dragonfly
Length of output: 1147
Crossbow charging is gated on the wrong item state.
Player.UseItem only reaches CanCharge after ReleaseCharge fails, which means the crossbow is still unloaded there. CanCharge currently requires !c.Item.Empty(), so the charging path never starts. Flip the condition to c.Item.Empty().
🐛 Proposed fix
func (c Crossbow) CanCharge(releaser Releaser, _ *world.Tx, ctx *UseContext) bool {
_, found := c.findProjectile(releaser, ctx)
- return found && !c.Item.Empty()
+ return found && c.Item.Empty()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // CanCharge ... | |
| func (c Crossbow) CanCharge(releaser Releaser, _ *world.Tx, ctx *UseContext) bool { | |
| _, found := c.findProjectile(releaser, ctx) | |
| return found && !c.Item.Empty() | |
| } | |
| // CanCharge ... | |
| func (c Crossbow) CanCharge(releaser Releaser, _ *world.Tx, ctx *UseContext) bool { | |
| _, found := c.findProjectile(releaser, ctx) | |
| return found && c.Item.Empty() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/item/crossbow.go` around lines 158 - 162, The Crossbow charging check
in CanCharge is using the wrong item state, so the charging path never starts
after Player.UseItem falls back from ReleaseCharge. Update CanCharge in Crossbow
to require the crossbow to be empty instead of non-empty, while still keeping
the projectile lookup via findProjectile and the found check.
Comment on lines
75
to
+79
| x, y, z := serverData["SpawnX"], serverData["SpawnY"], serverData["SpawnZ"] | ||
| if x == nil || y == nil || z == nil { | ||
| return cube.Pos{}, true, fmt.Errorf("error reading spawn fields from server data for player %v", id) | ||
| return world.PlayerSpawn{}, true, fmt.Errorf("error reading spawn fields from server data for player %v", id) | ||
| } | ||
| return cube.Pos{int(x.(int32)), int(y.(int32)), int(z.(int32))}, true, nil | ||
| spawn = world.PlayerSpawn{Pos: cube.Pos{int(x.(int32)), int(y.(int32)), int(z.(int32))}, Dim: world.Overworld} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded type assertions on SpawnX/Y/Z can panic on malformed data.
x, y, z are checked for nil but not asserted safely; x.(int32) etc. will panic if the stored value isn't int32 (e.g. corrupted or foreign level data). Contrast with the SpawnDimension handling two lines below, which uses the safe comma-ok form. Recommend applying the same guarded pattern here.
🛡️ Proposed fix
- x, y, z := serverData["SpawnX"], serverData["SpawnY"], serverData["SpawnZ"]
- if x == nil || y == nil || z == nil {
- return world.PlayerSpawn{}, true, fmt.Errorf("error reading spawn fields from server data for player %v", id)
- }
- spawn = world.PlayerSpawn{Pos: cube.Pos{int(x.(int32)), int(y.(int32)), int(z.(int32))}, Dim: world.Overworld}
+ x, xOk := serverData["SpawnX"].(int32)
+ y, yOk := serverData["SpawnY"].(int32)
+ z, zOk := serverData["SpawnZ"].(int32)
+ if !xOk || !yOk || !zOk {
+ return world.PlayerSpawn{}, true, fmt.Errorf("error reading spawn fields from server data for player %v", id)
+ }
+ spawn = world.PlayerSpawn{Pos: cube.Pos{int(x), int(y), int(z)}, Dim: world.Overworld}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| x, y, z := serverData["SpawnX"], serverData["SpawnY"], serverData["SpawnZ"] | |
| if x == nil || y == nil || z == nil { | |
| return cube.Pos{}, true, fmt.Errorf("error reading spawn fields from server data for player %v", id) | |
| return world.PlayerSpawn{}, true, fmt.Errorf("error reading spawn fields from server data for player %v", id) | |
| } | |
| return cube.Pos{int(x.(int32)), int(y.(int32)), int(z.(int32))}, true, nil | |
| spawn = world.PlayerSpawn{Pos: cube.Pos{int(x.(int32)), int(y.(int32)), int(z.(int32))}, Dim: world.Overworld} | |
| x, xOk := serverData["SpawnX"].(int32) | |
| y, yOk := serverData["SpawnY"].(int32) | |
| z, zOk := serverData["SpawnZ"].(int32) | |
| if !xOk || !yOk || !zOk { | |
| return world.PlayerSpawn{}, true, fmt.Errorf("error reading spawn fields from server data for player %v", id) | |
| } | |
| spawn = world.PlayerSpawn{Pos: cube.Pos{int(x), int(y), int(z)}, Dim: world.Overworld} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/world/mcdb/db.go` around lines 75 - 79, The spawn coordinate loading
in the db.go path is using unsafe type assertions for SpawnX, SpawnY, and
SpawnZ, so malformed server data can panic. Update the spawn parsing logic
around the existing serverData lookup and PlayerSpawn construction to use
guarded comma-ok assertions, matching the safer SpawnDimension handling already
used nearby, and return the same error path if any coordinate is missing or not
int32.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
High Risk
Changes player spawn persistence and cross-dimension respawn resolution, plus End crystal explosions and entity damage paths—core gameplay and world data behavior.
Overview
Adds End crystal placement item/entity behavior (End fire, beam/base metadata, vanilla-style explosions with optional block clipping below obsidian/bedrock and reduced entity damage underwater) and respawn anchors (glowstone charging, Nether spawn setting, charge depletion on respawn, overworld misuse explosion).
Player respawn now stores position + dimension (
PlayerSpawn), persistsSpawnDimensionin LevelDB, and resolves saved spawns via optionalWorldByDimensionon login/respawn; beds and anchors share a genericrespawnBlocksafe-spawn path.Also ships bamboo block/mosaic (separate from log/wood registration), bone meal outcomes (
BoneMealResult) driving small vs area particles (including grass spreading plants),HurtEntityso projectiles/melee can damage non-Livingentities like crystals, explosionDisableEntityDamage, rayBBoxIntersects/BlockIntersects, chunk clone APIs andHighestFilledSubChunksemantics fix, Sulfur Caves biome, extra debug primitive shapes, and dependency bumps (gophertunnel 1.57, worldupgrader 1.0.21).Reviewed by Cursor Bugbot for commit 556a712. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes